⚠️ GitHub issue #47099 has been automatically assigned in GitHub to PR creator.

Could you try to add following code to fix https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/52408405

diff --git a/cpp/src/parquet/exception.h b/cpp/src/parquet/exception.h
index 46f5595dd1..55b769c978 100644
--- a/cpp/src/parquet/exception.h
+++ b/cpp/src/parquet/exception.h
@@ -84,7 +84,7 @@
 
 namespace parquet {
 
-class ParquetException : public std::exception {
+class PARQUET_EXPORT ParquetException : public std::exception {
  public:
   PARQUET_NORETURN static void EofException(const std::string& msg = "") {
     static std::string prefix = "Unexpected end of stream";

Could you try to add following code to fix https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/52408405

I did that but it did not help much AFAICS. How do we continue? Should I add another warning push/pop to exception.h?

FAILED: [code=2] src/parquet/CMakeFiles/parquet_shared.dir/arrow/path_internal.cc.obj 
"C:\Program Files\Git\usr\bin\ccache.exe" C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\cl.exe  /nologo /TP -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_WITH_TIMING_TESTS -DHAVE_INTTYPES_H -DHAVE_NETDB_H -DNOMINMAX -DPARQUET_EXPORTING -DPARQUET_THRIFT_VERSION_MAJOR=0 -DPARQUET_THRIFT_VERSION_MINOR=20 -D_CRT_SECURE_NO_WARNINGS -D__SSE2__ -D__SSE4_1__ -D__SSE4_2__ -Dparquet_shared_EXPORTS -ID:\a\arrow\arrow\build\cpp\src -ID:\a\arrow\arrow\cpp\src -ID:\a\arrow\arrow\cpp\src\generated -external:ID:\a\arrow\arrow\build\cpp\vcpkg_installed\x64-windows\include -external:W0 /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING /W3 /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4365 /wd4267 /wd4838 /wd4800 /wd4996 /wd4065 /arch:SSE4.2  /Ob0 /Od /RTC1 /WX -std:c++17 -MDd -Zi /showIncludes /Fosrc\parquet\CMakeFiles\parquet_shared.dir\arrow\path_internal.cc.obj /Fdsrc\parquet\CMakeFiles\parquet_shared.dir\ /FS -c D:\a\arrow\arrow\cpp\src\parquet\arrow\path_internal.cc
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): error C2220: the following warning is treated as an error
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): warning C4275: non dll-interface class 'std::exception' used as base for dll-interface class 'parquet::ParquetException'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\vcruntime_exception.h(49): note: see declaration of 'std::exception'
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): note: see declaration of 'parquet::ParquetException'
[318/403] Building CXX object src\arrow\flight\CMakeFiles\arrow_flight_shared.dir\transport\grpc\protocol_grpc_internal.cc.obj
[319/403] Building CXX object src\parquet\CMakeFiles\parquet_shared.dir\arrow\reader.cc.obj
FAILED: [code=2] src/parquet/CMakeFiles/parquet_shared.dir/arrow/reader.cc.obj 
"C:\Program Files\Git\usr\bin\ccache.exe" C:\PROGRA~1\MICROS~2\2022\ENTERP~1\VC\Tools\MSVC\1444~1.352\bin\Hostx64\x64\cl.exe  /nologo /TP -DARROW_HAVE_RUNTIME_AVX2 -DARROW_HAVE_RUNTIME_AVX512 -DARROW_HAVE_RUNTIME_BMI2 -DARROW_HAVE_RUNTIME_SSE4_2 -DARROW_HAVE_SSE4_2 -DARROW_WITH_TIMING_TESTS -DHAVE_INTTYPES_H -DHAVE_NETDB_H -DNOMINMAX -DPARQUET_EXPORTING -DPARQUET_THRIFT_VERSION_MAJOR=0 -DPARQUET_THRIFT_VERSION_MINOR=20 -D_CRT_SECURE_NO_WARNINGS -D__SSE2__ -D__SSE4_1__ -D__SSE4_2__ -Dparquet_shared_EXPORTS -ID:\a\arrow\arrow\build\cpp\src -ID:\a\arrow\arrow\cpp\src -ID:\a\arrow\arrow\cpp\src\generated -external:ID:\a\arrow\arrow\build\cpp\vcpkg_installed\x64-windows\include -external:W0 /D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING /W3 /EHsc /wd5105 /bigobj /utf-8 /W3 /wd4365 /wd4267 /wd4838 /wd4800 /wd4996 /wd4065 /arch:SSE4.2  /Ob0 /Od /RTC1 /WX -std:c++17 -MDd -Zi /showIncludes /Fosrc\parquet\CMakeFiles\parquet_shared.dir\arrow\reader.cc.obj /Fdsrc\parquet\CMakeFiles\parquet_shared.dir\ /FS -c D:\a\arrow\arrow\cpp\src\parquet\arrow\reader.cc
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): error C2220: the following warning is treated as an error
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): warning C4275: non dll-interface class 'std::exception' used as base for dll-interface class 'parquet::ParquetException'
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.44.35207\include\vcruntime_exception.h(49): note: see declaration of 'std::exception'
D:\a\arrow\arrow\cpp\src\parquet/exception.h(87): note: see declaration of 'parquet::ParquetException'

From the above log, warning C4275 is fired but it should be disabled already via pragma warning(disable : 4275 4251). I guess the reason is that adding pragma warning(pop) to the parquet/platform.h header file will immediately swipe out all pushed pragmas at the end of this header file and any source file including parquet/platform.h will not see any pushed pragmas any more.

From the above log, warning C4275 is fired but it should be disabled already via pragma warning(disable : 4275 4251). I guess the reason is that adding pragma warning(pop) to the parquet/platform.h header file will immediately swipe out all pushed pragmas at the end of this header file and any source file including parquet/platform.h will not see any pushed pragmas any more.

Yes, that is also my understanding on how the push / pop mechanism works. The warnings are only disabled for everything between the push and the pop. So we either need to

1. fix the warning or
2. add another push/pop to exception.h or
3. add an include for exception.h to platform.h between the push and the pop so that exception.h won't be reread with the warnings being enabled.

I would prefer option 2 but I have only very limited knowledge about the code base.

Since parquet/platform.h is included by almost all files under parquet subdirectory, I don't think we need to add pop.

Please correct me if I was wrong. cc @kou @raulcd

Since parquet/platform.h is included by almost all files under parquet subdirectory, I don't think we need to add pop.

But if you don't add the pop, the warning is disabled for every file which does #include parquet/platform.h directly or indirectly. So basically every code file using parquet (not only inside the library but also users of the library). That was the whole point of the issue #47099 as far as I am concerned. If you want to disable the warning only for the CI build, you could achieve this with a command line option to the compiler and get rid of the push as well. But every push needs its pop :-)

As you can see, adding pop to the header file is not sufficient. Since ParquetException is used almost by all Parquet source files, we will end up with adding pop to every source file which seems meaningless if the warning can't be fixed.

you could achieve this with a command line option to the compiler and get rid of the push as well

Can we use this approach something like the following?

diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt
index dc7d40d2a3..717496496a 100644
--- a/cpp/src/parquet/CMakeLists.txt
+++ b/cpp/src/parquet/CMakeLists.txt
@@ -352,6 +352,18 @@ foreach(LIB_TARGET ${PARQUET_LIBRARIES})
   endif()
 endforeach()
 
+# Disable warnings
+foreach(LIB_TARGET ${PARQUET_LIBRARIES})
+  if(MSVC)
+    # Disable warning for STL types usage in DLL interface
+    # https://web.archive.org/web/20130317015847/http://connect.microsoft.com/VisualStudio/feedback/details/696593/vc-10-vs-2010-basic-string-exports
+    target_compile_options(${LIB_TARGET} PUBLIC "/wd4275" "/wd4251")
+    # Disable diamond inheritance warnings
+    target_compile_options(${LIB_TARGET} PRIVATE "/wd4250")
+    ...
+  endif()
+endforeach()
+
 if(WIN32 AND ARROW_BUILD_STATIC)
   target_compile_definitions(parquet_static PUBLIC PARQUET_STATIC)
 endif()

We can use PRIVATE for warnings that are only for Parquet source.
We can use PUBLIC for warnings that are for Parquet source and Parquet users.

As you can see, adding pop to the header file is not sufficient. Since ParquetException is used almost by all Parquet source files, we will end up with adding pop to every source file which seems meaningless if the warning can't be fixed.

I don't think so. We just need to add another pair of push/pop to exception.h. I can give this a try if you wish

Sure, feel free to try it out. Thanks!

I did, but the appveyor build failed for different reasons. I am not sure whether simply re-triggering it will already help.

We still see errors in https://github.com/apache/arrow/actions/runs/16593712171/job/46935525834?pr=47114

OK, I accidentally removed the PARQUET_EXPORT and now tried again

I am not sure about the cancelled ones but all Windows builds seem to be green now

@kou @wgtmac Thanks for the very helpful review! Is there still anything I can do to get it integrated?

Could you rebase on main to fix CI failures?

I did a rebase

Hmm. Could you rebase on https://github.com/apache/arrow/tree/main ?

https://github.com/behrisch/arrow/tree/patch-1 shows:

This branch is 191 commits ahead of, 183 commits behind apache/arrow:main.

It must not have "... commits behind" after the correct rebase.

Sorry, something went wrong on my side. I hope it is corrected now. @kou

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 349d232.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.